Fix race conditions between caching reader and worker (again) #2308
Conversation
|
On master the following debug assertion triggered while loading a track: |
|
I think we should hold off on releasing 2.2.3 for a bit until we're confident all these recent changes to CachingReader are working.
see #2235 |
|
The tests seem to fail because they don't keep an event loop running, although they expect signals to arrive. This does not work for queued connections. Looks like the test framework is insufficient for testing a correct implementation? At some point I have to stop, this is getting completely out of control. |
|
...and now MockedEngineBackendTest gets stuck. |
| // Handle signals from worker thread | ||
| connect(&m_worker, | ||
| &CachingReaderWorker::trackLoaded, | ||
| this, |
There was a problem hiding this comment.
Does this work without a direct connection?
Do we have a Qt Main Loop?
There was a problem hiding this comment.
If we don't have an event loop then we must not use signals. Period.
Can you recall which assertion has failed? I still see no non my system. Did it occur in the 2.2 or master branch?
|
| } | ||
|
|
||
| void CachingReader::process() { | ||
| ReaderStatusUpdate update; | ||
| while (m_stateFIFO.read(&update, 1) == 1) { | ||
| while (m_readerStatusUpdateFIFO.read(&update, 1) == 1) { | ||
| DEBUG_ASSERT(m_state != State::Idle); |
There was a problem hiding this comment.
This assertion may occur after ejecting a track and loading a new one.
Was this the one you have experienced?
There was a problem hiding this comment.
Added comments to the wrong/closed PR: #2305 (review)
This is the debug assertion that helped me discover the race condition. I refuse to remove it because it is correct unless someone provides a counterexample.
There was a problem hiding this comment.
I have checked the old version in 3e9e2ae before the fixes and I still don't see a reason why this debug assertion is not valid?
|
|
|
The whole design is broken and slots/signals are used incorrectly. CachingReader lives the main thread, receives signals from a worker thread and process()/read() are called from an engine thread. I surrender. |
Even more serious race conditions and design flaws.
I've also reverted some unintended renamings from the previous PR, sorry for the noise.